nix shell: Handle output paths that are symlinks #10467
Conversation
This requires moving resolveSymlinks() into SourceAccessor. Also, it requires LocalStoreAccessor::maybeLstat() to work on parents of the store (to avoid an error like "/nix is not in the store"). Fixes NixOS#10375.
github-actions
bot
added
new-cli
| if (isDirOrInDir(store->storeDir, path.abs())) | ||
| return Stat{ .type = tDirectory }; |
There was a problem hiding this comment.
It's not clear why this is needed (calling it with a parent of the store feels quite wrong).
I guess it should at least honor requireValidPath
There was a problem hiding this comment.
Because when following a symlink from one store path to another, resolveSymlinks() will stat the parents of those paths, so it will do a maybeLstat("/nix") and maybeLstat("/nix/store"). Those are not valid store paths so it failed.
There was a problem hiding this comment.
I've been thinking about whether stores' source accessors should prefix everything with the store directory or not, and leaning towards "no".
This would be something not doing that helps with: the path is just xxxxxxxxxxxx-foo, no parent directories etc.
There was a problem hiding this comment.
makeMountedInputAccessor can be used to put put things in store directories if it matters.
There was a problem hiding this comment.
#10510 Broke into new issue
There was a problem hiding this comment.
Given that #10510 isn't fixed, I think this is actually a fine fix for now.
edolstra
force-pushed
the
nix-shell-symlink
branch
from
d9a6421 to
26a4688
Compare
|
@thufschmitt Any other comments? Would be good to get this merged. |
| CanonPath SourceAccessor::resolveSymlinks( | ||
| const CanonPath & path, | ||
| SymlinkResolution mode) | ||
| { | ||
| auto res = CanonPath::root; | ||
|
|
||
| int linksAllowed = 1024; | ||
|
|
||
| std::list<std::string> todo; | ||
| for (auto & c : path) | ||
| todo.push_back(std::string(c)); | ||
|
|
||
| while (!todo.empty()) { | ||
| auto c = *todo.begin(); | ||
| todo.pop_front(); | ||
| if (c == "" || c == ".") | ||
| ; | ||
| else if (c == "..") | ||
| res.pop(); | ||
| else { | ||
| res.push(c); | ||
| if (mode == SymlinkResolution::Full || !todo.empty()) { | ||
| if (auto st = maybeLstat(res); st && st->type == SourceAccessor::tSymlink) { | ||
| if (!linksAllowed--) | ||
| throw Error("infinite symlink recursion in path '%s'", showPath(path)); | ||
| auto target = readLink(res); | ||
| res.pop(); | ||
| if (hasPrefix(target, "/")) | ||
| res = CanonPath::root; | ||
| todo.splice(todo.begin(), tokenizeString<std::list<std::string>>(target, "/")); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return res; | ||
| } |
There was a problem hiding this comment.
I think this can be deduped
There was a problem hiding this comment.
With the code in src/libutil/file-path-impl.hh
See the implementation of canonPath in src/libutil/file-system.cc
There was a problem hiding this comment.
That's not the job of this PR though. It just moves resolveSymlinks().
There was a problem hiding this comment.
I think this is OK for now, but I would really like Eelco to take on #10510 / #9852 / #9892 (comment) so we have a more robust solution for this stuff
|
Oops, replied to this in the wrong PR! #10511 (comment) is what I meant. I still think we should merge this as-is. |
|
Would be great to have this backported. |
thufschmitt
added
the
backport 2.21-maintenance
|
Successfully created backport PR for |
Uuuuh, I though this had arrived after the release. Created the backport, should be merged once the CI is green |
Motivation
This requires moving resolveSymlinks() into SourceAccessor. Also, it requires LocalStoreAccessor::maybeLstat() to work on parents of the store (to avoid an error like "/nix is not in the store").
Fixes #10375.
Context
Priorities and Process
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.